subsys: usb: host: Add USB Host Vendor specific serial class support#99173
subsys: usb: host: Add USB Host Vendor specific serial class support#99173Girinandha-M wants to merge 4 commits intozephyrproject-rtos:mainfrom
Conversation
1b5bbd4 to
5c2f40a
Compare
|
Hello @Girinandha-M, the fixes from #99334 got integrated into #94590. If you wished to simplify your PR you may be able to switch to #94590. |
There was a problem hiding this comment.
While this is a simple and efficient API, there is an existing API in Zephyr for serial lines, which for instance USB CDC ACM supports, and which might be needed in the final version of this driver.
@jfischer-no @tmon-nordic
Should this be handled as a UART async API? As UART IRQ-based API? Custom one like now?
I think the goal is to get modem used, so both would work, I suppose something like this on top of it:
https://github.com/zephyrproject-rtos/zephyr/tree/3cf7b202780d788976e4bf3e46724e63d23f6bcc/drivers/modem
There was a problem hiding this comment.
Implementing UART API would probably be the easiest solution to get higher-level things working on top of such USB serial port. I am not sure what design sacrifices would have to be made to support UART API though.
There was a problem hiding this comment.
Thanks for the feedback — agreed on reusing the existing Zephyr serial/UART APIs.
The async UART API was chosen specifically to match modem-style usage (non-blocking TX/RX)
Please let me know if there are concerns with using the async UART API
5c2f40a to
37d1b8b
Compare
|
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
37d1b8b to
9d4d294
Compare
|
|
@Girinandha-M the dependencies should be merged now. Can you take this out of draft and rebase on top of |
| # Copyright (c) 2026 Linumiz | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| description: | | ||
| USB Host Vendor class serial support. | ||
|
|
||
| compatible: "zephyr,usbh-serial" | ||
|
|
||
| include: base.yaml |
There was a problem hiding this comment.
Devicetree should not be used for class driver instantiation.
| @@ -0,0 +1,64 @@ | |||
| /* | |||
There was a problem hiding this comment.
What is the purpose of this header file and why is it exposed to all users?
| # | ||
|
|
||
| config USBH_SERIAL | ||
| bool "USB Host Vendor-Specific Serial Support" |
There was a problem hiding this comment.
What vendor? Is it intended to touch this code every time a new "Vendor" needs to be supported? It does not make sense to have this code in the tree parallel to CDC ACM.
There was a problem hiding this comment.
From discussions on Discord, this is the counterpart of this Linux driver:
With various vendor-specific extras, some device not having any vendor specifics listed there:
There was a problem hiding this comment.
I guess you mean imitation not counterpart. I do not think it is something to imitate or use as reference.
There was a problem hiding this comment.
@jfischer-no @josuah Thanks for your feedback.
My goal with this change was to support the Quectel EG916Q modem, which exposes a vendor-specific bulk interface used for AT/PPP communication.
I initially looked at how Linux handles similar devices via the option driver and attempted a more generic “vendor serial” class approach to potentially support multiple modems in the future.
However, I understand the concern about introducing a parallel framework to CDC ACM and potentially imitating Linux’s usb-serial model.
For Zephyr, would it be preferable to:
- Implement this as a device-specific driver (matching VID/PID) under a modem driver instead of a generic vendor class?
- Integrate this functionality differently within the existing USB host class structure?
or is there any other different approach should i look into ?
There was a problem hiding this comment.
Link for convenience: ./drivers/usb/serial/option.c
|
This pull request has been marked as stale because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 7 days. Note, that you can always re-open a closed pull request at any time. |
9d4d294 to
adfc3a7
Compare
|
Rebased on main, reworked based on feedback:
Found and fixed a upstream HS enumeration bugs in usbh_device.c during testing (separate commit). |
adfc3a7 to
507b574
Compare
josuah
left a comment
There was a problem hiding this comment.
Here is some very incomplete review, in the meantime that a more complete review can be done.
| struct vendor_serial_quirk { | ||
| uint16_t vid; | ||
| uint16_t pid; | ||
| uint8_t at_iface; |
There was a problem hiding this comment.
Is it really AT interface? This driver is expected to be a generic serial, so network-specific vocabulary might be best taken away from the shared bits?
| static int submit_tx(struct vendor_serial_data *data, | ||
| const uint8_t *buf, size_t len) | ||
| { | ||
| struct uhc_transfer *xfer; | ||
| struct net_buf *nbuf; | ||
| int ret; | ||
|
|
||
| xfer = usbh_xfer_alloc(data->udev, data->bulk_out_ep, tx_cb, data); | ||
| if (!xfer) { | ||
| return -ENOMEM; | ||
| } | ||
|
|
||
| nbuf = usbh_xfer_buf_alloc(data->udev, len); | ||
| if (!nbuf) { | ||
| usbh_xfer_free(data->udev, xfer); | ||
| return -ENOMEM; | ||
| } | ||
|
|
||
| memcpy(nbuf->data, buf, len); | ||
| net_buf_add(nbuf, len); | ||
|
|
||
| ret = usbh_xfer_buf_add(data->udev, xfer, nbuf); | ||
| if (ret) { | ||
| usbh_xfer_buf_free(data->udev, nbuf); | ||
| usbh_xfer_free(data->udev, xfer); | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
Grouping several feedback points by taking inspiration of other code in USB codebase, that can be propagated elsewhere as well:
| static int submit_tx(struct vendor_serial_data *data, | |
| const uint8_t *buf, size_t len) | |
| { | |
| struct uhc_transfer *xfer; | |
| struct net_buf *nbuf; | |
| int ret; | |
| xfer = usbh_xfer_alloc(data->udev, data->bulk_out_ep, tx_cb, data); | |
| if (!xfer) { | |
| return -ENOMEM; | |
| } | |
| nbuf = usbh_xfer_buf_alloc(data->udev, len); | |
| if (!nbuf) { | |
| usbh_xfer_free(data->udev, xfer); | |
| return -ENOMEM; | |
| } | |
| memcpy(nbuf->data, buf, len); | |
| net_buf_add(nbuf, len); | |
| ret = usbh_xfer_buf_add(data->udev, xfer, nbuf); | |
| if (ret) { | |
| usbh_xfer_buf_free(data->udev, nbuf); | |
| usbh_xfer_free(data->udev, xfer); | |
| return ret; | |
| } | |
| static int submit_tx(cons struct device *const dev, | |
| const uint8_t *const buf, const size_t len) | |
| { | |
| struct vendor_serial_data *const data = dev->data; | |
| struct uhc_transfer *xfer; | |
| struct net_buf *nbuf; | |
| int ret; | |
| xfer = usbh_xfer_alloc(data->udev, data->bulk_out_ep, tx_cb, buf); | |
| if (xfer == NULL) { | |
| return -ENOMEM; | |
| } | |
| nbuf = usbh_xfer_buf_alloc(data->udev, len); | |
| if (nbuf == NULL) { | |
| ret = -ENOMEM; | |
| goto some_label; | |
| } | |
| net_buf_add_mem(nbuf, buf, len); | |
| ret = usbh_xfer_buf_add(data->udev, xfer, nbuf); | |
| if (ret != 0) { | |
| goto some_label; | |
| } |
net_buftypically has utilities for most data movement- const everywhere possible
- use
const struct device *devandstruct usbh_class_data *c_dataas first parameter everywhere, and dispatch thedataetc. parameters when needed (using*consttoo), possibly because it helps - use explicit
if (... == NULL)andif (... == 0)to help with MISRA - use centralized exit using
gotofor repeated error handling
This is just my interpretation of other code, hopefully it is accurate and you are not asked to roll back any of it... feel free to discuss any point too.
| static const struct vendor_serial_quirk quirk_table[] = { | ||
| { | ||
| .vid = QUECTEL_VENDOR_ID, | ||
| .pid = QUECTEL_EG916Q_PID, | ||
| .at_iface = 2, | ||
| .desc = "Quectel EG916Q-GL", | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Did you have to skip the first interface? Would an heuristic that seeks the first interface with a pair of BULK endpoints work?
.desc sounds great for debugging, but risks to grow a huge table of strings as more devices are added, and bloat flash usage.
How about just logging the VID:PID? Impacts debuggability but still reasonable.
There was a problem hiding this comment.
The EG916Q exposes 4 vendor-class interfaces (0-3), all with identical bInterfaceClass=0xff, bInterfaceSubClass=0x00, bInterfaceProtocol=0x00.
Interface 0 is diagnostics, 1 is NMEA, 2 is AT, 3 is PPP, but descriptors are indistinguishable. A heuristic picking the first bulk pair would claim interface 0 (diagnostics), which is wrong.
The current design uses a three-way probe:
- Quirk match (VID+PID+iface) - claim specific interface.
- Known device, unlisted interface - skip (prevents heuristic from grabbing the wrong port).
- Unknown device - heuristic, first bulk pair selected.
This keeps the quirk table small (only devices with ambiguous interfaces need entries) while unknown single-interface vendor devices work with zero configuration.
There was a problem hiding this comment.
0 is diagnostics, 1 is NMEA, 2 is AT, 3 is PPP
So there should be 4 serial interfaces rather than just one?
As this is not described as a driver for AT protocol anywhere.
It might be an user choice to decide which interface to access and for which purpose?
Someone might want to access the other kinds of command interfaces some day.
This means there would need one dev per serial line, each with its own serial dev->api.
So far, const struct device * for USB hosts are statically allocated in advance.
This suggests you would need another LISTIFY to generate the const struct device *.
In addition to the existing LISTIFY, which would only generate the struct usbh_class_data *c_data.
And then some dynamic logic would allow to map the interfaces to use depending on each device. Such as some kind of filtering. Maybe giving a flag to each interface (i.e. AT, NMEA, PPP, DEBUG), and the user could have a callback to take a decision of skipping or binding a particular endpoint pair?
The goal being removing the policy out of the drivers, but instead just fully document each hardware connected and expose the information to users.
507b574 to
64d7d75
Compare
There was a problem hiding this comment.
I think the first three commits are worthwhile to have in main even before there is agreement on how to proceed with "usb: host: add USB Serial support". Can you move them to separate PR? Ah you already posted them separately in #106802
| } | ||
|
|
||
| err = usbh_req_desc_dev(udev, sizeof(udev->dev_desc), &udev->dev_desc); | ||
| err = alloc_device_address(udev, &new_addr); |
There was a problem hiding this comment.
While this is OK from the interoperability point of view (allowing to work with broken devices), what exactly does "matching USB 2.0 enumeration sequence" refer to?
64d7d75 to
a772dd4
Compare
Set default bMaxPacketSize0 to 64 before reading the device descriptor. HS EP0 is required to use 64-byte packets per USB 2.0 spec (section 5.5.3), and the current default of 8 causes the host controller to program EP0 with mps=8, which rejects the 64-byte response from any HS device as a babble error. For FS the actual mps0 is unknown until the device descriptor is read, but 64 is a safe upper bound for the initial 8-byte read since wLength fits in a single packet at any FS mps0. Signed-off-by: Santhosh Charles <santhosh@linumiz.com>
Move alloc_device_address() and usbh_device_set_address() before reading the full device descriptor. The current sequence reads the full 18-byte device descriptor at address 0 before assigning a device address. This causes enumeration failure on the Quectel EG916Q-GL which returns bNumConfigurations=0 on the second GET_DESCRIPTOR at address 0. reorder to assign the address after the initial 8-byte read and before the full descriptor read. Both orderings are permitted by the USB 2.0 spec, but the new order is strictly more compatible and matches the enumeration sequence used by Linux (hub_port_init) and xHCI host stacks. Signed-off-by: Girinandha Manivelpandiyan <girinandha@linumiz.com>
usbh_desc_get_next_function() sets skip_num=1 when starting from a plain interface descriptor. usbh_desc_get_next() already advances past the input, so an additional skip_num=1 makes the loop consume one extra interface beyond the current one. Devices with consecutive plain interfaces (e.g. Quectel EG916Q-GL with 4 vendor interfaces) only get ifaces 0 and 2 probed; 1 and 3 are silently skipped. Drop the assignment. The IAD branch is unaffected since its skip_num comes from bInterfaceCount, which counts interfaces grouped inside the IAD, not the IAD descriptor itself. Signed-off-by: Girinandha Manivelpandiyan <girinandha@linumiz.com>
add support for the vendor specific serial support class to the usb subsys. This implementation enable the usb host class support the vendor specific devices for serial communication. Signed-off-by: Girinandha Manivelpandiyan <girinandha@linumiz.com>
a772dd4 to
86bd18b
Compare
|



Dependency:
Upstream Reference:
subsys: usb: host: add vendor-specific serial class
Add USB vendor-specific serial host class driver that matches
devices by VID, PID.
This is not a standard CDC-ACM driver - it's for vendor-specific
serial devices that use bulk endpoints instead of CDC protocol.
Signed-off-by: Girinandha Manivelpandiyan girinandha@linumiz.com